Add reporting feature to GitHub webhook-based taskspawner#966
Open
kelos-bot[bot] wants to merge 3 commits intomainfrom
Open
Add reporting feature to GitHub webhook-based taskspawner#966kelos-bot[bot] wants to merge 3 commits intomainfrom
kelos-bot[bot] wants to merge 3 commits intomainfrom
Conversation
This enables status reporting back to GitHub issues/PRs for tasks spawned via webhook events, achieving feature parity with the polling-based GitHub issue and pull request sources. Changes: - Add Reporting field to GitHubWebhook CRD struct - Stamp reporting annotations on webhook-created tasks when enabled - Add reporting reconciler to webhook server binary - Update reportingEnabled() to check webhook sources - Regenerate CRD manifests and deepcopy Co-Authored-By: Claude Opus 4.6 <[email protected]>
github-actions
Bot
added
needs-triage
needs-kind
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmd/kelos-spawner/main.go">
<violation number="1" location="cmd/kelos-spawner/main.go:484">
P1: Webhook reporting is still effectively disabled: this new `GitHubWebhook` check is bypassed by `sourceAnnotations` returning early for webhook sources, so webhook-created tasks won't receive reporting annotations.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if ts.Spec.When.GitHubPullRequests != nil && ts.Spec.When.GitHubPullRequests.Reporting != nil { | ||
| return ts.Spec.When.GitHubPullRequests.Reporting.Enabled | ||
| } | ||
| if ts.Spec.When.GitHubWebhook != nil && ts.Spec.When.GitHubWebhook.Reporting != nil { |
There was a problem hiding this comment.
P1: Webhook reporting is still effectively disabled: this new GitHubWebhook check is bypassed by sourceAnnotations returning early for webhook sources, so webhook-created tasks won't receive reporting annotations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/kelos-spawner/main.go, line 484:
<comment>Webhook reporting is still effectively disabled: this new `GitHubWebhook` check is bypassed by `sourceAnnotations` returning early for webhook sources, so webhook-created tasks won't receive reporting annotations.</comment>
<file context>
@@ -481,6 +481,9 @@ func reportingEnabled(ts *kelosv1alpha1.TaskSpawner) bool {
if ts.Spec.When.GitHubPullRequests != nil && ts.Spec.When.GitHubPullRequests.Reporting != nil {
return ts.Spec.When.GitHubPullRequests.Reporting.Enabled
}
+ if ts.Spec.When.GitHubWebhook != nil && ts.Spec.When.GitHubWebhook.Reporting != nil {
+ return ts.Spec.When.GitHubWebhook.Reporting.Enabled
+ }
</file context>
…gedPredicate The reportingAnnotationPredicate watches Task status phase changes directly. GenerationChangedPredicate is removed to avoid reconcile churn from annotation updates that bump metadata.generation. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Collaborator
|
/kelos pick-up |
The webhook check in reportingEnabled() was unreachable in the spawner context because sourceAnnotations() returns early for webhook sources. Webhook reporting annotations are stamped directly by the webhook handler, not the spawner. Remove the dead code and its tests. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Collaborator
|
/kelos review |
1 similar comment
Collaborator
|
/kelos review |
![kelos-bot[bot]](https://avatars.githubusercontent.com/in/2965305?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: APPROVE (submitted as comment — cannot self-approve)
Scope: Adds GitHub status reporting to webhook-based TaskSpawners via annotation stamping and a new reporting reconciler in the webhook server
Findings
Tests (non-blocking)
cmd/kelos-webhook-server/reporting.go— No test file exists for this new file. The reconciler,reportingAnnotationPredicate, andreadGitHubTokenare untested. The handler-side tests (internal/webhook/handler_test.go) are thorough and cover annotation stamping, disabled reporting, PR events, and source kind mapping — but the webhook server's reporting controller should also be tested, at minimum forreadGitHubTokenedge cases (file not found, empty file, env var fallback) and the predicate filtering logic.
Code Quality (non-blocking)
cmd/kelos-webhook-server/reporting.go:101vscmd/kelos-spawner/main.go:641—readGitHubTokenis duplicated with subtle behavioral differences: (1) the webhook version correctly falls back toGITHUB_TOKENwhen the token file is empty, while the spawner version returns an empty string; (2) the webhook version omits the file path from the error format string ("reading token file: %w"vs"reading token file %s: %w"). Consider extracting to a shared function ininternal/reporting/to avoid this divergence.
Correctness (nit)
internal/webhook/handler.go:503—webhookSourceKindincludes"pull_request_target"in the switch. This is a GitHub Actions trigger concept, not a standard GitHub webhook event type (the webhook API haspull_request,pull_request_review,pull_request_review_comment, andpull_request_review_thread). It's harmless since no real webhook would deliver this event type, but it's slightly misleading.
Verification
make verify— passesmake test— all tests in changed packages pass (internal/webhookOK,cmd/kelos-spawnerOK)- Generated files (deepcopy, CRD YAMLs) are included and up to date
- PR description follows the template, includes a meaningful release note for the new user-facing feature
Suggestions
- Add unit tests for
readGitHubTokenandreportingAnnotationPredicatein a newcmd/kelos-webhook-server/reporting_test.go - Extract the shared
readGitHubTokenintointernal/reporting/to avoid the subtle behavioral divergence between copies - Consider adding the annotation check to
reportingAnnotationPredicate.Createto avoid unnecessary reconciles for tasks without reporting annotations (the current approach works correctly but reconciles and discards all non-reporting task creates)
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds GitHub status reporting support for webhook-based TaskSpawners, achieving feature parity with the polling-based GitHubIssues and GitHubPullRequests sources.
When
reporting.enabled: trueis set on aGitHubWebhooksource, the webhook server will:kelos.dev/github-reporting,kelos.dev/source-kind,kelos.dev/source-number) on newly created TasksThe implementation follows the existing annotation-driven reporting pattern — the reporting infrastructure is source-agnostic and only requires the correct annotations to be present on the Task.
Which issue(s) this PR is related to:
Fixes #958
Special notes for your reviewer:
--github-owner,--github-repo,--github-token-file, and--github-api-base-urlflags to enable the reporting controller. If owner/repo are not provided, the reporting controller is not started.webhookSourceKind()helper correctly maps webhook event types to the existing"issue"/"pull-request"annotation values. Forissue_commentevents on PRs, it checks thePullRequestAPIURLfield.Number(push events, for example, are skipped).Does this PR introduce a user-facing change?
Summary by cubic
Adds GitHub status reporting to webhook-based TaskSpawners so tasks spawned from webhook events post and update status comments on the originating issue or PR, matching polling sources.
New Features
spec.when.githubWebhook.reporting.enabled: true.kelos.dev/github-reporting,kelos.dev/source-kind, andkelos.dev/source-numberon tasks.cmd/kelos-webhook-server, gated by--github-owner,--github-repo,--github-token-file, and--github-api-base-url.issueorpull-request(e.g.,issue_commenton PRs maps topull-request); skips events without a number (e.g.,push).reportingEnabled()is scoped to polling sources (webhook reporting is handled by the webhook server).Migration
reporting.enabled: trueonGitHubWebhooksources and run the webhook server with the GitHub flags; provide a token via--github-token-fileorGITHUB_TOKEN.Written for commit 7a9816b. Summary will update on new commits.